fix(react-query): hydrate deferred queries before observers subscribe to avoid a redundant refetch - #11137
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesHydrationBoundary hydration timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Loader
participant HydrationBoundary
participant QueryCache
participant QueryComponent
Loader->>HydrationBoundary: Provide dehydrated query state
HydrationBoundary->>QueryCache: Hydrate queued query in layout effect
QueryComponent->>QueryCache: Subscribe during remount
QueryCache-->>QueryComponent: Return hydrated data
QueryComponent->>QueryCache: Do not invoke client query function
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6829c4d to
fe46639
Compare
ostapondo
changed the title
|
Updated after review feedback on #11157. Before, this moved the whole queue to a layout effect, existing queries included, which also changed when already-mounted observers see hydrated data (commit time instead of after paint) and needed one assertion in an existing test to be relaxed. Only the unobserved half needs to move to fix the refetch, so that's all it does now. The test file is additions only, nothing existing changed, and "Existing observer should not have updated at this point" keeps guarding the observed case. Same shape in #11157 now. |
fe46639 to
fca2f4c
Compare
ostapondo
changed the title
|
Rolled the split back out of this one, it doesn't hold up in React. React cleans up a removed subtree's subscriptions in the passive phase, after this layout effect runs. So when a page unmounts and remounts in the same commit, which is what happens as soon as the boundary wraps the page, the outgoing observer is still counted on the query, the split skips it and the refetch is back. Added a test for that shape, it fails with the split and passes with this. So this is back to hydrating the whole queue in the layout effect, which is fine here because React never commits a render it throws away. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-query/src/__tests__/HydrationBoundary.test.tsx`:
- Around line 152-160: Update the HydrationBoundary test’s existing-observer
assertions so the prior value remains visible during render, and only the
hydrated value appears after passive effects flush. Keep the immediate “added”
assertion for newly introduced query data, then advance timers and assert the
existing query changes to “should change” afterward.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bd29617-79dc-47a3-a8a6-86fe60bd2a2c
📒 Files selected for processing (3)
.changeset/hungry-planes-tease.mdpackages/react-query/src/HydrationBoundary.tsxpackages/react-query/src/__tests__/HydrationBoundary.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/hungry-planes-tease.md
- packages/react-query/src/HydrationBoundary.tsx
… to avoid a redundant refetch
fca2f4c to
ab12b64
Compare
1 participant
🎯 Changes
Fixes #9610.
HydrationBoundarydefers hydration of queries that already exist in the cache to an effect, so that transitions don't update existing observers mid-render. ButuseQuery'suseSyncExternalStoresubscription is also a passive effect, and children's effects run before parents'. On a revisit, where the query is cached but was unmounted, the observer subscribes first, sees the old stale entry and starts a refetch of the exact data the dehydrated state already contains. The server computes it in the loader and the client immediately fetches it again.This moves the deferred hydration, and the
optionsRefsync it reads, to a layout effect. All layout effects run before any passive effect, so hydration lands in the cache before a remounting observer decides whether to fetch. Layout effects still only run when the tree commits, so the aborted transition behaviour is unchanged and its test stays green.Behaviour change worth noting: already-mounted observers now get hydrated data at commit time rather than after paint. One timing assertion in an existing test was updated for that. The render phase guarantee is still covered by the aborted transition test.
I tried a narrower version that only hydrates queries with no subscribers, so mounted observers keep their old timing. It doesn't work here: React cleans up a deleted subtree's subscriptions in the passive phase, after this layout effect, so when a page unmounts and remounts in the same commit the outgoing observer is still on the query and the refetch comes back. There's a test for that case now.
Verified with tests that fail on
mainand pass with the fix, plus a jsdom repro of the React Router loader flow from the issue against the built package.Not covered: a
useSuspenseQueryover a query that is inpendingstate with no data starts its fetch during render, before any layout effect. That pre-existing path is unaffected by this change.✅ Checklist
pnpm run test:pr.🚀 Release Impact
🤖 Generated with Claude Code
Summary by CodeRabbit